-
Notifications
You must be signed in to change notification settings - Fork 39
[김인] sprint2 #153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[김인] sprint2 #153
The head ref may contain hidden characters: "Basic-\uAE40\uC778-sprint2"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
전체적으로 포맷팅이랑 CSS 파일 구조화해보시면서 리팩토링하시면 도움 많이 되실것같아요 👍
주요 리뷰 포인트
- 포맷팅
- CSS 파일 구조화
login.html
Outdated
| </header> | ||
| <!-- body --> | ||
| <body> | ||
| <div class = "login_signup_container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <div class = "login_signup_container"> | |
| <div class="login_signup_container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런식으로 속성명과 값 사이에 공백 넣어주실 필요없으니 제거해보실래요?
포맷팅이 잘못되어있으면 전체적인 코드 퀄리티를 눈에 보이게 저하시켜서 좋은 포맷팅 습관을 들일수있도록 지금은 수동으로 고치고, 나중에 prettier로 자동 포맷팅 적용시켜보시면 좋을것같아요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가적으로 스프린트 미션2라서 따로 코멘트 안드리려했는데 코드가 포함되어있어서 말씀드리자면, div 태그보다는 form 태그를 사용하는건어떨까요?
의미론적으로도 올바르고, (Semantic)
접근성 향상 및 사용성 개선에도 도움이 됩니다.
우선 form태그는 웹페이지에서 사용자 입력을 서버로 전송하기위해 사용하는 html 요소인데, 특정 url로 전송하거나 http method를 지정할수있습니다.
그리고 기본적인 유효성검사를 지원하는데, 폼 요소에 required 속성을 사용하면 필수 입력 조건이 걸려서 비워서 제출할수없게끔 만들어줄수있어요.
물론 지금 서버 제출을 고려하지않고 작업을 하고있지만, 일반적으로 폼 요소는 서버에 제출하기위해 쓰이니까 알아두시는게 좋겠죠? 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 스타일을 해당 파일에서 써주는것보다는 CSS 파일또한 용도를 나눠서 구조화하면 훨씬 유지보수에 용이하답니다.
용도에 따라 CSS를 나눠서 관리해볼까요?
- reset.css: CSS 초기화
- common.css: 전역 스타일
- login.css, signup.css: 페이지별 로컬 스타일
이렇게 용도에 따라 파일을 나눠 관리하는건 어떤 이점을 가져다줄수있는지 한번 더 생각해볼수있어요.
- 역할 분리가 되어 관리가 쉬움: 각 파일의 목적이 명확해서 수정이나 유지보수가 쉬워질거예요.
- 사용성과 확장성 증가: 여러 페이지 혹은 프로젝트에서 거의 동일하게 재사용이 가능해요.
- 스타일 우선순위가 더 예측 가능해짐: reset.css → common.css → 페이지나 컴포넌트별 스타일 순으로 로딩하면 우선순위가 명확해지고 스타일 충돌을 줄일 수 있어요.
style.css
Outdated
| } | ||
|
|
||
| } | ||
| @media (max-width: 1920px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
브레이크포인트를 max-width: 1920px 하나만 쓰게되면 나중에 반응형 레이아웃 구성하실때 다 바꿔주셔야할텐데 기본 스타일은 모바일에 맞추고, (작은 화면에서부터) 큰 화면으로 점차 확장해나가는 순서로 작성하는 방향으로 리팩토링해볼까요? 이렇게 하면 불필요한 스타일 재정의 및 코드 중복을 효과적으로 줄일 수 있답니다 :)
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게